Fix incorrect `#bind_call` monkey patch #1002

amomchilov · 2023-07-27T19:03:40Z

Description

Ruby/debug monkey patches in an implementation of UnboundMethod#bind_call for compatibility with Rubies older than 2.7. Unfortunately, there is a bug in the implementation, in that it doesn't pass the block parameter correctly. Here's a demo:

$ chruby 2.6.10 && ruby ./bind_call_bug_demo.rb
Backporting UnboundMethod#bind_call in Ruby 2.6.10.

Incorrect result from buggy bind_call: nil
  Correct result from fixed bind_call: #<Proc:0x00000001100df230@./bind_call_bug_demo.rb:12>
                        Expected proc: #<Proc:0x00000001100df230@./bind_call_bug_demo.rb:12>

$ chruby 3.2.2 && ruby ./bind_call_bug_demo.rb
Ruby 3.2.2 already has UnboundMethod#bind_call natively defined.

Correct result from native bind_call: #<Proc:0x00000001055ab5f8 ./bind_call_bug_demo.rb:12>
                       Expected proc: #<Proc:0x00000001055ab5f8 ./bind_call_bug_demo.rb:12>

bind_call_bug_demo.rb

require "test/unit"

class SampleTarget < Struct.new(:id)
  def pass_block_through(&block)
    block
  end
end

target_object = SampleTarget.new

m = SampleTarget.instance_method(:pass_block_through)
proc = Proc.new { |x| x }


if UnboundMethod.method_defined?(:bind_call)
  puts "Ruby #{RUBY_VERSION} already has UnboundMethod#bind_call natively defined.\n\n"

  puts "Correct result from native bind_call: #{m.bind_call(target_object, &proc).inspect}" # => #<Proc:...>
  puts "                       Expected proc: #{proc}"  # => #<Proc:...>
else
  puts "Backporting UnboundMethod#bind_call in Ruby #{RUBY_VERSION}.\n\n"

  class UnboundMethod
    def bind_call(receiver, *args)
      bind(receiver).call(*args)
    end

    def bind_call_fixed(receiver, *args, &block)
      bind(receiver).call(*args, &block)
    end
  end

  puts "Incorrect result from buggy bind_call: #{m.bind_call(target_object, &proc).inspect}" # => nil
  puts "  Correct result from fixed bind_call: #{m.bind_call_fixed(target_object, &proc).inspect}" # => #<Proc:...>
  puts "                        Expected proc: #{proc}"  # => #<Proc:...>
end

This repo introduces a corrected patch, and with a test that confirms it:

# for Ruby 2.6 compatibility
unless UnboundMethod.method_defined?(:bind_call)
  class UnboundMethod
    def bind_call(receiver, *args, &block)
      bind(receiver).call(*args, &block)
    end
  end
end

I also introduces a new Reflection module which extracts all the bind_call-ed reflection methods, so they're more readable. It also adds tests for them.

amomchilov · 2023-07-27T19:09:18Z

lib/debug/reflection.rb

+    module_function def instance_variables_of(o)
+      M_INSTANCE_VARIABLES.bind_call(o)
+    end


Connecting the debugger to a Ruby version pre-2.7 will have the effect of back-porting the bind_call method, which would otherwise exist.

This is convenient for the implementation of the Ruby/debug itself, but perhaps it's not a good idea, as developers' code can call bind_call successfully, but only when they're debugging, which could be quite confusing.

Although it's more tedious, perhaps it's a better idea to conditionally define one of two implementations of these helpers, rather than polluting UnboundMethod? E.g.

Suggested change

module_function def instance_variables_of(o)

M_INSTANCE_VARIABLES.bind_call(o)

end

if UnboundMethod.method_defined?(:bind_call)

module_function def instance_variables_of(o)

M_INSTANCE_VARIABLES.bind_call(o)

end

else

module_function def instance_variables_of(o)

M_INSTANCE_VARIABLES.bind(o).call

end

end

I'm not sure we should support pre-2.7 though...

If we don't want to support pre-2.7, then we should remove the monkey patch entirely.

I don't personally use anything pre-3.2, but it feels too soon to drop 2.7. In any case, the current implementation of it is incorrect, and we should not leave it as-is.

amomchilov · 2023-07-27T19:13:39Z

lib/debug/server_cdp.rb

@@ -1059,8 +1059,8 @@ def process_cdp args
                result = b.local_variable_get(expr)
              rescue NameError
                # try to check method
-                if M_RESPOND_TO_P.bind_call(b.receiver, expr, include_all: true)


Interestingly, respond_to?'s include_all argument is positional, not keyword (notice =, not : in the docs):

respond_to?(symbol, include_all=false) → true or false

The way this is called causes an implicit Hash to be passed positionally, as if it was:

if M_RESPOND_TO_P.bind_call(b.receiver, expr, { include_all: true })

This would cause include_all's value to be literally the Hash { include_all: true }. It's truthy, so it behaves equivalent to true, so it happens to be accidentally correct. This wouldn't work though:

if M_RESPOND_TO_P.bind_call(b.receiver, expr, include_all: false)

Since the { include_all: false } Hash would still be truthy.

I made this a kwarg in the Reflection helper, so it's more clear.

amomchilov · 2023-07-27T19:16:19Z

test/debug/reflection_test.rb

+      omit_if(
+        UnboundMethod.instance_method(:bind_call).source_location.nil?,
+        "This Ruby version (#{RUBY_VERSION}) has a native #bind_call implementation, so it doesn't need the backport.",
+      )


This causes a fair bit of noise in the test output:

================================================================================================================================================= Omission: This Ruby version (3.2.2) has a native #bind_call implementation, so it doesn't need the backport. [test_bind_call_backport(DEBUGGER__::ReflectionTest)] ./test/debug/reflection_test.rb:60:in `test_bind_call_backport' ================================================================================================================================================= Finished in 0.001056 seconds. ------------------------------------------------------------------------------------------------------------------------------------------------- 10 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 1 omissions, 0 notifications

Perhaps it would be better to just wrap the def test_bind_call_backport in a big if statement?

amomchilov · 2023-07-27T19:16:41Z

test/debug/reflection_test.rb

+      assert_equal "parg2", result.fetch(:parg2)
+      assert_equal rest_args, result.fetch(:rest_args)
+      assert_equal kwargs, result.fetch(:kwargs)
+      assert_same proc, result.fetch(:block)


The buggy patch would cause this to be nil, but it now works correctly.

ko1 · 2023-09-25T07:43:08Z

The name "Reflection" is not clear for me. Reflection is important programming technique and debugger should support it especially on Ruby. However, this module doesn't represent this kind of general idea.
Could you make another word?

amomchilov · 2023-09-26T22:08:22Z

@ko1 I think Reflection is exactly the right term. Have a look at the Wikiepdia page:

Discover and modify source-code constructions (such as code blocks, classes, methods, protocols, etc.) as first-class objects at runtime.

Convert a string matching the symbolic name of a class or function into a reference to or invocation of that class or function.

ko1 · 2023-09-27T01:20:49Z

Yes, this module uses "Reflection" feature, but this module doesn't represent the feature of "Reflection".

amomchilov · 2023-09-27T17:35:56Z

@ko1 Sorry, I don't think I understand.

I suppose ReflectionHelpers/ReflectionUtils would be more precise names, but *Helpers names are smelly. It reads better at the callsite, e.g. the standard library gives you Math.sin not MathHelpers.sin.

This naming also what Tapioca calls it.

If you would like an alternative name, could you suggest one? I don't have any better ideas.

ko1 · 2023-10-02T07:36:09Z

lib/debug/reflection.rb

+
+module DEBUGGER__
+  module Reflection
+    module_function def instance_variables_of(o)


why do you use module_function?

If the module were included, it could override kernel methods like #responds_to?, which wouldn't be desirable. So these methods should be called directly on the module.

Would you prefer I use include self instead?

ko1 · 2023-10-02T07:59:50Z

If you would like an alternative name, could you suggest one? I don't have any better ideas.

This module provides a feature to call essential Ruby methods so

EssentialRubyFeature
PrimitiveRubyFeature
OriginalRubyFeature

and so on...?

amomchilov · 2023-10-02T14:55:23Z

Personally, I think those are too generic.

___RubyFeature raises the question: "Which Ruby features are essential, primitive or original?"

The answer is: "the reflection-related features"

I really do think Reflection is the exact right term for this, but it's more important to me that this lands in any form. Please feel free to use whichever you find best.

ko1 · 2023-10-09T08:24:23Z

___RubyFeature raises the question: "Which Ruby features are essential, primitive or original?"
The answer is: "the reflection-related features"

I understand the misunderstanding with us.

You understand these methods are "reflection related features"
I understand these methods are guaranteed to be original methods.

Original methods mean that they are guaranteed to provide original feature even if they are re-defined by other classes.

For example, if we get trouble with Array#each redefinition, I think Array.instance_method(:each) should be in this module. But maybe you don't think so.

Hmm.

amomchilov · 2023-10-10T00:12:33Z

Hi Koichi! You're correct, I think this is exactly the misunderstanding.

For example, if we get trouble with Array#each redefinition, I think Array.instance_method(:each) should be in this module. But maybe you don't think so.

Correct! I was thinking of it more by purpose ("reflection related features") than implementation detail ("it's an original Ruby method, ignoring redefinition"). If I wanted the original Array#each, I would put it in some new array-specific module which better represents some relevant high-level idea (e.g. “Enumeration”)

I'm okay with either I approach. Please feel free to merge whichever you prefer.

ko1 · 2023-10-19T09:30:26Z

I also misunderstand that you introduced different naming like

    module_function def instance_variable_get_from(o, name)
      M_INSTANCE_VARIABLE_GET.bind_call(o, name)
    end

(not instance_variable_get).

ko1 · 2023-10-19T09:38:31Z

OK, How about ObjectIntrospector?

@mametter told me that
https://stackoverflow.com/questions/25198271/what-is-the-difference-between-introspection-and-reflection#answer-41161878

here we need the ability for "Introspection"

Fix incorrect bind_call backport and extract Reflection methods

0213f32

amomchilov force-pushed the reflection-helpers branch from 3d0eebb to 0213f32 Compare July 27, 2023 19:04

amomchilov commented Jul 27, 2023

View reviewed changes

amomchilov marked this pull request as ready for review July 27, 2023 20:43

ko1 reviewed Oct 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect `#bind_call` monkey patch #1002

Fix incorrect `#bind_call` monkey patch #1002

amomchilov commented Jul 27, 2023

amomchilov Jul 27, 2023

ko1 Sep 25, 2023

amomchilov Sep 26, 2023

amomchilov Jul 27, 2023

amomchilov Jul 27, 2023

amomchilov Jul 27, 2023

ko1 commented Sep 25, 2023

amomchilov commented Sep 26, 2023

ko1 commented Sep 27, 2023

amomchilov commented Sep 27, 2023

ko1 Oct 2, 2023

amomchilov Oct 2, 2023 •

edited

Loading

ko1 commented Oct 2, 2023

amomchilov commented Oct 2, 2023 •

edited

Loading

ko1 commented Oct 9, 2023

amomchilov commented Oct 10, 2023 •

edited

Loading

ko1 commented Oct 19, 2023

ko1 commented Oct 19, 2023 •

edited

Loading

Fix incorrect #bind_call monkey patch #1002

Are you sure you want to change the base?

Fix incorrect #bind_call monkey patch #1002

Conversation

amomchilov commented Jul 27, 2023

Description

amomchilov Jul 27, 2023

Choose a reason for hiding this comment

ko1 Sep 25, 2023

Choose a reason for hiding this comment

amomchilov Sep 26, 2023

Choose a reason for hiding this comment

amomchilov Jul 27, 2023

Choose a reason for hiding this comment

amomchilov Jul 27, 2023

Choose a reason for hiding this comment

amomchilov Jul 27, 2023

Choose a reason for hiding this comment

ko1 commented Sep 25, 2023

amomchilov commented Sep 26, 2023

ko1 commented Sep 27, 2023

amomchilov commented Sep 27, 2023

ko1 Oct 2, 2023

Choose a reason for hiding this comment

amomchilov Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

ko1 commented Oct 2, 2023

amomchilov commented Oct 2, 2023 • edited Loading

ko1 commented Oct 9, 2023

amomchilov commented Oct 10, 2023 • edited Loading

ko1 commented Oct 19, 2023

ko1 commented Oct 19, 2023 • edited Loading

Fix incorrect `#bind_call` monkey patch #1002

Fix incorrect `#bind_call` monkey patch #1002

amomchilov Oct 2, 2023 •

edited

Loading

amomchilov commented Oct 2, 2023 •

edited

Loading

amomchilov commented Oct 10, 2023 •

edited

Loading

ko1 commented Oct 19, 2023 •

edited

Loading